Skip to content
This repository has been archived by the owner on Oct 24, 2021. It is now read-only.

Use chai and sinon from npm module #415

Closed
wants to merge 1 commit into from
Closed

Conversation

trajano
Copy link

@trajano trajano commented May 3, 2016

Thanks for submitting a PR! We'll try to look at it as soon as possible.

If you are adding significant new content, please take a moment to include an update to the changelog in your PR.

Use this field to describe your pull request.

Rather than practicalmeteor:chai, use the npm versions. This allows us to
ensure we can get the latest versions possible of the libraries and allows
us to do tests using other test packages.

See practicalmeteor/meteor-mocha#23 and https://github.com/xolvio/meteor-jasmine/issues/334#issuecomment-208725856 for other reasons.

@apollo-cla
Copy link

@trajano: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@tmeasday
Copy link
Contributor

tmeasday commented May 3, 2016

Thanks @trajano! This seems like a good idea -- could I ask that you send the equivalent PR to https://github.com/meteor/todos so we can double check that it works?

@tmeasday
Copy link
Contributor

@trajano any chance of that PR?

@trajano
Copy link
Author

trajano commented Jun 29, 2016

Sorry no, RL work took priority. Plus there's a few errors after I upgraded meteor on the boilerplate code that I need to address first.

@abernix
Copy link
Contributor

abernix commented Sep 30, 2016

Any updates on this @trajano ?

@abernix
Copy link
Contributor

abernix commented Mar 10, 2017

@trajano No worries if you're still busy, but just checking back on this! Did you encounter any insurmountable problems with switching to this? Seems like a reasonable idea overall.

@trajano
Copy link
Author

trajano commented Mar 10, 2017

I don't recall having any problems when I did it back then, but Meteor has been evolving since I created the PR. No guarantees, but if I get some free time today I'll take a quick look.

Rather than practicalmeteor:chai, use the npm versions.  This allows us to
ensure we can get the latest versions possible of the libraries and allows
us to do tests using other test packages.

See practicalmeteor/meteor-mocha#23 and https://github.com/xolvio/meteor-jasmine/issues/334#issuecomment-208725856 for other reasons.
@trajano
Copy link
Author

trajano commented Mar 12, 2017

DO NOT MERGE YET. I still have to make sure things work, I just rebased it for now.

@trajano
Copy link
Author

trajano commented Mar 12, 2017

Hmm like I said I haven't played with Meteor for a while, I just tried to do meteor test --driver-package practicalmeteor:mocha on Windows and got

D:\m\todos>meteor npm install
...

D:\m\todos>meteor run
... works welll ... ctrl-C

D:\m\todos>meteor test --driver-package practicalmeteor:mocha
C:\Users\trajano\AppData\Local\.meteor\packages\meteor-tool\1.4.2_3\mt-os.windows.x86_32\dev_bundle\lib\node_modules\meteor-promise\promise_server.js:190
      throw error;
      ^

Error: EPERM: operation not permitted, symlink 'C:\Users\trajano\AppData\Local\.meteor\packages\meteor-tool\1.4.2_3\mt-os.windows.x86_32\dev_bundle\server-lib\node_modules\' -> 'C:\Users\trajano\AppData\Local\Temp\meteor-test-rund2fckq\.meteor\local\build\programs\server\node_modules'
    at Error (native)
    at Object.fs.symlinkSync (fs.js:897:18)
    at Object.wrapper (C:\tools\fs\files.js:1535:35)
    at Object.files.cp_r (C:\tools\fs\files.js:501:11)
    at C:\tools\fs\files.js:488:13
    at Array.forEach (native)
    at Object.files.cp_r (C:\tools\fs\files.js:475:25)
    at C:\tools\fs\files.js:488:13
    at Array.forEach (native)
    at Object.files.cp_r (C:\tools\fs\files.js:475:25)
    at C:\tools\fs\files.js:488:13
    at Array.forEach (native)
    at Object.files.cp_r (C:\tools\fs\files.js:475:25)
    at copyDirIntoTestRunnerApp (C:\tools\cli\commands.js:1692:15)
    at doTestCommand (C:\tools\cli\commands.js:1698:5)
    at Command.func (C:\tools\cli\commands.js:1533:10)
    at C:\tools\cli\main.js:1483:23

From a clone of the todos app. Going to try doing a meteor update to see if it will help. However, even after meteor update and meteor update --all-packages No luck as of yet.

From the stack trace it looks like it is a fundamental problem of Meteor itself rather than the driver since all the errors are from the C:\tools or native

@kfern
Copy link

kfern commented Mar 13, 2017

Hi. Could you try open console as admin?

@trajano
Copy link
Author

trajano commented Mar 13, 2017

@kfern thanks that got it working. I'll continue when I can later. Is this new I don't recall ever having to do this before (running as admin I mean)

@abernix
Copy link
Contributor

abernix commented Mar 16, 2017

We really do not want anyone having to run meteor as Administrator as it's almost always caused problems somewhere along the line (on both Windows and Unix).

I think it's worth considering a recommendation to use dispatch:mocha here. If you have the opportunity to test, please do – the instructions on their GitHub seem to work for me. It's recently received more updates than the practicalmeteor implementation and breaks free of being locked into PhantomJS, which has become more and more problematic these days, it seems.

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for checking into this, @trajano.

Marking as "Needs changes" based on the previous "don't merge" exclamation. This is just to avoid accidental merging – happy to dismiss this review as deemed appropriate. 😉

@trajano
Copy link
Author

trajano commented Mar 16, 2017

@abernix I'll look into this dispatch:mocha you speak of then maybe I'll just change it around to use that one instead.

@abernix
Copy link
Contributor

abernix commented Sep 12, 2017

Any updates on this, @trajano?

@trajano
Copy link
Author

trajano commented Sep 12, 2017

Closing this off, I don't have time to work on this anymore sorry.

@trajano trajano closed this Sep 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants